Enable internal rustc lints
#316
Conversation
It has too many false-positives to be useful, unfortunately.
BD103
added
A-Build-System
|
@BD103 Are you sure we want to enable |
Actually, no, I didn't see that conversation before. I think some lints may still be useful, but maybe it would be good to avoid enabling the entire If you look at the list of lints, which ones do you think are most useful for us? |
BD103
added
S-Needs-Investigation
Hmm, good question. I'm fine with just enabling all and seeing how it feels? Otherwise, these sound most useful to me:
|
I think I'm fine with this as well. We'll experiment with them all on (except (Also that link on incremental compilation was really cool!) |
BD103
added
S-Needs-Review
|
|
||
| #[allow( | ||
| rustc::potential_query_instability, | ||
| reason = "Iterating a hash map may lead to query instability, but the fix is not trivial." |
There was a problem hiding this comment.
could we swap to a BTreeMap?
There was a problem hiding this comment.
Great idea! I didn't even think of that! I switched it over to BTreeMap. Looks like rustc_data_structures has SortedMap<K, V>, which may be better, but I'll stick to BTreeMap for now.
DaAlbrecht
left a comment
DaAlbrecht
left a comment
There was a problem hiding this comment.
What an interesting issue this has been! Learned a lot :)
Turns out our
#![warn(rustc::internal)]wasn't doing anything, since-Z unstable-optionswasn't being passed to the compiler. This PR enables the internalrustclints in CI, and leaves a comment so people know how to enable it themselves.With the lint group now enabled, this PR also fixes any leftover warnings. The biggest note right now is that we iterate over a hash map in some places, even though that iteration is not deterministic. There was one spot that I temporarily
#[allow(...)]d, even though it wasn't deterministic, since it seemed to complicated to fix right now.